Display node 75% view submenus #64121

bkimmel · 2020-04-21T21:16:54Z

Summary

This PR adds submenu options to Resolver nodes according to the mocks supplied below.

Notes:

Combined the two menu host buttons into 1 more general component. This makes the code easier to read (1 component instead of 2) and more flexible. ( https://github.com/elastic/kibana/pull/64121/files#diff-b18117271821fdd985eed1e1c4fd749fR75 )
During development, it was noted that the EUI components we are using conflict with the mocks in a few places (e.g. divider lines between options). Where there are conflicts, we are generally planning to break towards the EUI component for consistency with the rest of the Kibana ecosystem. We'll make minimal non-obtrusive adjustments like ( https://github.com/elastic/kibana/pull/64121/files#diff-b18117271821fdd985eed1e1c4fd749fR482 ) for integration with the comprehensive design.
After conversation with the team, we decided that as a short-term measure to allow demo/testing we're going to "spool" all the related event stats from the backend. This obviously will not work long-term for production, as it defeats the purpose of limiting/paging the /related API. Issue: https://github.com/elastic/endpoint-app-team/issues/379
Type information was added to provide a basic shape for the way related data will be tendered to the Resolver ( 36fe414 ).
After further discussion with ( @james-elastic @jonathan-buttner @kqualters-elastic ) a decision was reached to reverse course on Populate scope with country name, not only country codes. #3 in favor of delaying the request for related event info until the submenu is opened (with an interstitial loading state being presented in place of the submenu items with counts). See discussion on issue ( https://github.com/elastic/endpoint-app-team/issues/379 ) for details.
Reducing the stats as a Map<ResolverEvent, StatsInfo> into DataState . Should be relatively fast to update and retrieve, and easy to remove (I've marked a few placed with comments indicating what can be excised easily) when the underlying issue forcing this workaround is resolved. Planning to have the selector merge the Map on top of the waiting Symbols for each result.
Requested pull of [ https://github.com/add related event generation to ancestor nodes (fixes a bug) #64950 ] to fix problem with related event generation for "ancestor" nodes.
Ancestor nodes problem is fixed
Completed the circuit from the API through the middleware => reducer => selector for a possible demo.
Fixed up as unknown as types in middleware here ( 4ad185d#diff-047733952e6965542d27766fbd28864bL102 )
11 ) Cleanup: moved the submenu to its own component file, added actions to be handled later for related alert/event requests.
Added test coverage for related event selectors and created an issue to explore reinforcing the test suite further ( https://github.com/elastic/endpoint-app-team/issues/418 ).
[As of 5/18/20] This PR stands with the requisite 2 eng and 1 design approvals. All comments have been adjusted for in commits and all outstanding questions answered.

Screenshots

current:

Mocks

MOCKS ONLY - THESE ARE HERE FOR DESIGN TO REFERENCE

MOCKS ONLY - THESE ARE HERE FOR DESIGN TO REFERENCE

Checklist

Delete any items that are not applicable to this PR.

[x ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
[x ] Unit or functional tests were updated or added to match the most common scenarios
[x ] This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
[x ] This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-04-27T14:04:37Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

This reverts commit ab58823.

…ubmenu-options

x-pack/plugins/endpoint/public/embeddables/resolver/store/actions.ts

x-pack/plugins/endpoint/public/embeddables/resolver/store/data/action.ts

oatkiller · 2020-05-18T14:23:01Z

x-pack/plugins/endpoint/public/embeddables/resolver/types.ts

+ * about a particular subject's related events
+ */
+export interface RelatedEventDataEntry {
+  relatedEvents: Array<{


why does this type have a relatedEvents key? In other words, why not:

export type RelatedEventDataEntry = Array<{relatedEvent: ResolverEvent; relatedEventType: string; }>;

I thought this would make the logic/types/etc. easier to integrate after the PR that fixes the underlying issue (which is currently under review) lands. I'm open to changing it if you feel it would make this more readable.

oatkiller · 2020-05-18T14:25:08Z

x-pack/plugins/endpoint/public/embeddables/resolver/types.ts

+export interface RelatedEventDataEntry {
+  relatedEvents: Array<{
+    relatedEvent: ResolverEvent;
+    relatedEventType: string;


If relatedEventType is calculated from ResolverEvent, why is this field needed? Couldn't the code that handles this action could just call eventType(relatedEvent)? This way, the action couldn't have a relatedEventType that was incorrect based on its relatedEvent, and the action would be more normalized.

The overall idea was to make it easier to read when the stats/counts get compiled. At one point this was covered by the union of possible types (which I removed at your request in an earlier commit), but I think I see what you mean there. I made an attempt at putting a more precise description around relatedEventType there. Is that OK, or should I just pull it off altogether?

7750c97#diff-2148112719bd69dd10f32e94d769f77eR141

x-pack/plugins/endpoint/common/models/event.ts

x-pack/plugins/endpoint/public/embeddables/resolver/view/submenu.tsx

… into resolver/lay-in-submenu-options

bkimmel · 2020-05-18T17:51:25Z

@oatkiller I believe all your suggestions have been implemented and I put answers to the balance of questions. Thank you for the review and please let me know if there are more changes to suggest or questions I can answer. 🙇

oatkiller · 2020-05-18T18:12:31Z

x-pack/plugins/endpoint/public/embeddables/resolver/store/data/reducer.ts

+    if (statsMap) {
+      const currentStatsMap = new Map(statsMap);
+      const [resolverEvent] = action.payload;
+      currentStatsMap.set(resolverEvent, new Error('error requesting related events'));


Is the error message being shown to the user? If not, I think having it in the code is confusing. If so, it should be translated, and it should probably be defined in the view part of the app.

oatkiller · 2020-05-18T18:24:49Z

x-pack/plugins/endpoint/public/embeddables/resolver/types.ts

 /**
 * State for `data` reducer which handles receiving Resolver data from the backend.
 */
 export interface DataState {
  readonly results: readonly ResolverEvent[];
  isLoading: boolean;
  hasError: boolean;
+  resultsEnrichedWithRelatedEventInfo?: Map<ResolverEvent, RelatedEventDataResults>;


Since resultsEnrichedWithRelatedEventInfo is optional, all the code that deals w/ it checks for its presence. When reading the code, I have to consider what would happen if the value is missing. When writing comprehensive tests, we would also have to account for scenarios where the value is missing.

However, the implementation always provides the value. If some code is written like:

if (resultsEnrichedWithRelatedEventInfo) { } else { // imagine code here }

Then any code in the else block is in fact dead code, as resultsEnrichedWithRelatedEventInfo isn't really optional. In the future, developers reading and maintaining this code will have to figure out that all these checks are pointless. They will have to figure out that the value isn't really optional. They may be confused, and think that the model is more complex than it is. They may assume that this value is optional, and that all code needs to deal w/ that fact.

I think for these reasons, you should mark the field as non-optional.

bkimmel · 2020-05-18T19:19:18Z

@oatkiller

Is the error message being shown to the user? If not, I think having it in the code is confusing. If so, it should be translated, and it should probably be defined in the view part of the app.

It was only intended to represent an error to the reader/debugger. I've adjusted per your recommendation ( 9e07c11 ) and in the future, I'll avoid using Errors to represent errors to avoid confusion.

bkimmel · 2020-05-18T19:30:33Z

@oatkiller

I think for these reasons, you should mark the field as non-optional.

I see your point - removing that allowed a bit of boilerplate to be taken out as well which I think, on balance, outweighs any benefits I was imagining about creating some arbitrary separation around that piece of state.

Fixed in ( 002d631 )

oatkiller

Thanks for addressing my complaints. Excited to see this in action

bkimmel · 2020-05-18T20:08:01Z

@oatkiller Thank you for taking the time to review. This PR stands with 2 engineering approvals ( yourself and @kqualters-elastic ) and 1 design ( @lindseypoli ). Hoping to merge soon, but I'll defer a bit to see if anyone has additional feedback.

x-pack/plugins/endpoint/public/embeddables/resolver/types.ts

kibanamachine · 2020-05-18T21:31:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 002d631

History

💚 Build #48754 succeeded 9e07c11
💚 Build #48733 succeeded 7750c97
💚 Build #48713 succeeded 7e85044
💛 Build #48122 was flaky f39f4cd
💔 Build #48068 failed 7e03a25

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* master: (24 commits) [APM] agent config 'profiling_inferred_spans_min_duration' default value is '0ms' but the min value in the field is '1ms' (elastic#66886) [Canvas] Fix flaky custom element functional tests (elastic#65908) Fix IE specific flexbox min-height issue (elastic#66555) [Discover] Unskip doc link functional test (elastic#66884) Index pattern management to Kibana platform (elastic#65026) Warning and link to support matrix for IE11 (elastic#66512) [Reporting] Consolidate Server Type Defs, move some out of Legacy (elastic#66144) [SIEM] [Maps] Fixes Network Map empty tooltip (elastic#66828) [Endpoint] Encode the index of the alert in the id response (elastic#66919) [services/testSubjects] reduce retry usage, add waitForEnabled (elastic#66538) [DOCS] Identifies cloud settings for APM (elastic#66935) [SIEM][CASE] Fix configuration's page user experience (elastic#66029) Resolver: Display node 75% view submenus (elastic#64121) [SIEM] Cases] Capture timeline click and open timeline in case view (elastic#66327) [APM] Lowercase agent names so icons work (elastic#66824) [dev/cli] add support for --no-cache (elastic#66837) [Ingest Manager] Better handling of package installation problems (elastic#66541) [ML] Enhances api docs for modules endpoints (elastic#66738) dont hide errors (elastic#66764) [RFC] Global search API (elastic#64284) ...

bkimmel · 2020-05-19T21:27:15Z

x-pack/plugins/endpoint/public/embeddables/resolver/store/data/reducer.ts

@@ -42,7 +42,7 @@ export const dataReducer: Reducer<DataState, ResolverAction> = (state = initialS
    if (statsMap) {
      const currentStatsMap = new Map(statsMap);
      const resolverEvent = action.payload;
-      currentStatsMap.set(resolverEvent, new Error('error requesting related events'));


📏 Don't: Use Errors to represent errors. Do: Use a string or an object

bkimmel · 2020-05-19T21:29:31Z

x-pack/plugins/endpoint/public/embeddables/resolver/types.ts

@@ -184,7 +184,7 @@ export interface DataState {
  readonly results: readonly ResolverEvent[];
  isLoading: boolean;
  hasError: boolean;
-  resultsEnrichedWithRelatedEventInfo?: Map<ResolverEvent, RelatedEventDataResults>;
+  resultsEnrichedWithRelatedEventInfo: Map<ResolverEvent, RelatedEventDataResults>;


📏 Don't: Mark things as optional (even if it seems they could/should be) unless you have code that uses them that way. Do: Mark most things as required in most cases.

Brent Kimmel added 5 commits April 21, 2020 17:12

cleanup: move submenus into a general component

c4ab6e2

remove unnecessary useState import

cfd95e8

referral for help

2c7b279

Use EUISelectable for options list

5aef930

adding EUISelectable

8ea107d

bkimmel added Feature:Endpoint Elastic Endpoint feature Feature:Resolver Security Solution Resolver feature labels Apr 27, 2020

Brent Kimmel added 6 commits April 27, 2020 12:46

user-select off to fix bug Jonny found

ab58823

Revert "user-select off to fix bug Jonny found"

42c9a3b

This reverts commit ab58823.

straighten corners on menu open to match mocks

f166d6d

adding type information for related events

6cb887b

refine type information for related events

36fe414

Node option submenus take waiting prop

2429b4b

bkimmel changed the title ~~cleanup: move submenus into a general component~~ Display node 75% view submenus Apr 28, 2020

Brent Kimmel added 15 commits April 28, 2020 17:50

Merge remote-tracking branch 'upstream/master' into resolver/lay-in-s…

41f4a79

…ubmenu-options

merge upstream

8c454d4

stub selector for related events

747cd6e

provision component with selected relatedEvents

72665e0

shape reducder

a98cd7a

adjust reducer for consistency

6e75074

adding request logic to middleware

5529b7e

investigating problems with /events and CLI

29e0018

pulling through selector

c7c22bf

test rendering in component

5631113

cleanup unused

e8d48db

more cleanup

01ea4a1

cleanup action

c64d990

fix submenu options typings

c7edee7

move utility function off and seal it

2d1b3f2

oatkiller suggested changes May 18, 2020

View reviewed changes

Brent Kimmel added 11 commits May 18, 2020 11:52

R Austin review: undo Memos, add comments

f601477

Merge remote-tracking branch 'origin/resolver/lay-in-submenu-options'…

3d52b8c

… into resolver/lay-in-submenu-options

R Austin review: adjust type on submenu for readability

1985a31

R Austin review: move selectable options to state hook

91f2bd6

R Austin review: simplify type, remove memo, remove inert return

c05de64

R Austin review: add comments, move testing instantiation

5bb0411

R Austin review: remove unnecessary type check

7e85044

R Austin review: direct return

c06da13

R Austin review: Add comments to actions

e742cce

R Austin review: change payload for failed action

fe978f6

R Austin review: More specific related type?

7750c97

oatkiller suggested changes May 18, 2020

View reviewed changes

R Austin review: Remove Error construction

9e07c11

R Austin review: mark relatedEventInfo non-optional

002d631

oatkiller approved these changes May 18, 2020

View reviewed changes

dplumlee reviewed May 18, 2020

View reviewed changes

x-pack/plugins/endpoint/public/embeddables/resolver/types.ts Show resolved Hide resolved

bkimmel merged commit 1b1a0f4 into elastic:master May 18, 2020

bkimmel deleted the resolver/lay-in-submenu-options branch May 18, 2020 21:34

bkimmel mentioned this pull request May 19, 2020

[7.x] Resolver: Display node 75% view submenus (#64121) #66951

Merged

bkimmel added a commit that referenced this pull request May 19, 2020

Resolver: Display node 75% view submenus (#64121) (#66951)

80052f7

bkimmel commented May 19, 2020

View reviewed changes

bkimmel mentioned this pull request Jun 15, 2020

[Endpoint] use rbush to only render to DOM resolver nodes that are in view #68957

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display node 75% view submenus #64121

Display node 75% view submenus #64121

bkimmel commented Apr 21, 2020 •

edited

Loading

elasticmachine commented Apr 27, 2020

oatkiller May 18, 2020

bkimmel May 18, 2020

oatkiller May 18, 2020

bkimmel May 18, 2020

bkimmel May 18, 2020

bkimmel commented May 18, 2020

oatkiller May 18, 2020

oatkiller May 18, 2020

bkimmel commented May 18, 2020

bkimmel commented May 18, 2020

oatkiller left a comment

bkimmel commented May 18, 2020

kibanamachine commented May 18, 2020

bkimmel May 19, 2020

bkimmel May 19, 2020

Display node 75% view submenus #64121

Display node 75% view submenus #64121

Conversation

bkimmel commented Apr 21, 2020 • edited Loading

Summary

Notes:

Screenshots

Mocks

Checklist

For maintainers

elasticmachine commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkimmel commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkimmel commented May 18, 2020

bkimmel commented May 18, 2020

oatkiller left a comment

Choose a reason for hiding this comment

bkimmel commented May 18, 2020

kibanamachine commented May 18, 2020

💚 Build Succeeded

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkimmel commented Apr 21, 2020 •

edited

Loading